Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Server capabilities support #4472

Merged
merged 17 commits into from
Jan 30, 2019
Merged

Server capabilities support #4472

merged 17 commits into from
Jan 30, 2019

Conversation

neilisfragile
Copy link
Contributor

@neilisfragile neilisfragile commented Jan 25, 2019

Implements MSC1753 and MSC1804.

@neilisfragile neilisfragile changed the title track unstable room v3 Room capabilities support MSC1804 Jan 25, 2019
@codecov-io
Copy link

codecov-io commented Jan 25, 2019

Codecov Report

Merging #4472 into develop will increase coverage by 0.05%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    #4472      +/-   ##
===========================================
+ Coverage    74.69%   74.75%   +0.05%     
===========================================
  Files          336      338       +2     
  Lines        34293    34650     +357     
  Branches      5592     5668      +76     
===========================================
+ Hits         25614    25901     +287     
- Misses        7095     7149      +54     
- Partials      1584     1600      +16

@neilisfragile neilisfragile requested a review from a team January 25, 2019 11:39
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good otherwise

@@ -118,6 +119,7 @@ class RoomVersions(object):
RoomVersions.V2,
RoomVersions.VDH_TEST,
RoomVersions.STATE_V2_TEST,
RoomVersions.V3,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't add V3 here yet, because doing so will allow us to accept joins/creates for V3 rooms, but won't actually implement the things that make V3 V3.

@turt2live turt2live requested a review from a team January 28, 2019 21:13
@turt2live
Copy link
Member

(sticking this back into the queue because I need it for Riot - sorry if I've picked the wrong person or something)

Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This actually doesn't work as is. The servlet needs registering (see dbc8f50) and MSC1753 requires authentication on the endpoint.

For bonus points, supporting m.change_password would be awesome.

@turt2live turt2live removed the request for review from a team January 28, 2019 21:51
@turt2live
Copy link
Member

Implementation for matrix-org/matrix-spec-proposals#1804

@turt2live turt2live dismissed their stale review January 29, 2019 12:58

Concerns addressed

@neilisfragile neilisfragile changed the title Room capabilities support MSC1804 Server capabilities support MSC1753 MSC1804 Jan 29, 2019
@richvdh richvdh requested a review from a team January 29, 2019 17:10
@richvdh richvdh changed the title Server capabilities support MSC1753 MSC1804 Server capabilities support Jan 29, 2019
"state-v2-test": "unstable",
}
},
"m.change_password": change_password,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

err, spec wants this to be {} at a minimum, not a boolean directly. I think we should modify the MSC to have this be the capability:

"m.change_password": {
  "enabled": true
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Member

@erikjohnston erikjohnston left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a few nits

@@ -104,6 +104,7 @@ class ThirdPartyEntityKind(object):
class RoomVersions(object):
V1 = "1"
V2 = "2"
V3 = "3"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it matters, but I'm a bit surprised this is still in here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is now updated due to #4515

},
}
})
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be nicer if you could move this out of the defer.returnValue, i.e. something like:

response = {
    ...
}

defer.returnValue((200, response))

it just makes it a lot easier to see what's going on

(200, {
"capabilities": {
"m.room_versions": {
"default": "1",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should pull this in from synapse.api.constants

@erikjohnston erikjohnston merged commit 6587b0b into develop Jan 30, 2019
turt2live added a commit to matrix-org/matrix-spec-proposals that referenced this pull request Jan 31, 2019
Original proposals:
* #1753
* #1804

Implementation proof:
* matrix-org/synapse#4472
* matrix-org/matrix-js-sdk#830

There is one change to MSC1753 which is included in this commit. MSC1804 remains unchanged. In the original proposal, the change password capability being present was an indication that password changes were possible. It was found that this doesn't really communicate the state very well to clients in that lack of a capability (or a 404, etc) would mean that users would erroneously not be able to change their passwords. A simple boolean flag was added to assist clients in detecting this capability.
@DMRobertson DMRobertson deleted the neilj/room_capabilities branch June 28, 2022 11:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants